Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Lock-free Queue #253

Merged

Conversation

saikishor
Copy link
Member

Fixes #14

@codecov-commenter
Copy link

codecov-commenter commented Jan 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.67%. Comparing base (db891ef) to head (f764bc8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
+ Coverage   70.12%   71.67%   +1.55%     
==========================================
  Files           8        9       +1     
  Lines         492      519      +27     
  Branches       84       85       +1     
==========================================
+ Hits          345      372      +27     
  Misses         92       92              
  Partials       55       55              
Flag Coverage Δ
unittests 71.67% <100.00%> (+1.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
include/realtime_tools/lock_free_queue.hpp 100.00% <100.00%> (ø)

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I don't fully understand all the C++ specifics here 😬
But the API seems to be clean from a user perspective, looking at the tests.

  • Does this replace the RealtimeBuffer? Or should the RealtimeBuffer be rewritten to use this queue with capacity of 1?
  • What is the practical difference between the RealtimeBox and the queue with capacity of 1? Maybe this is clear for some, but I'd need (and appreciate) a user guide when to use what.
  • If we deprecate something, we can also add migration notes to this repository and link it on control.ros.org

package.xml Outdated Show resolved Hide resolved
include/realtime_tools/lock_free_queue.hpp Outdated Show resolved Hide resolved
include/realtime_tools/lock_free_queue.hpp Outdated Show resolved Hide resolved
test/lock_free_queue_tests.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested that with ros-controls/ros2_controllers#1480, the API works for me!

@saikishor
Copy link
Member Author

I tested that with ros-controls/ros2_controllers#1480, the API works for me!

@christophfroehlich I had to rewrite some parts of tests to reuse rather than copying for every instance. I've add more tests for all constructable types

package.xml Show resolved Hide resolved
@bmagyar bmagyar merged commit 1d66fed into ros-controls:master Jan 29, 2025
19 checks passed
@saikishor saikishor deleted the boost/lockfree/realtime/buffer branch January 29, 2025 18:41
@saikishor saikishor added backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS 2 humble. backport-jazzy This label should be used by maintainers only! Label triggers PR backport to ROS 2 humble. labels Jan 29, 2025
mergify bot pushed a commit that referenced this pull request Jan 29, 2025
(cherry picked from commit 1d66fed)
mergify bot pushed a commit that referenced this pull request Jan 29, 2025
(cherry picked from commit 1d66fed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS 2 humble. backport-jazzy This label should be used by maintainers only! Label triggers PR backport to ROS 2 humble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lock-free buffer implementation
5 participants